-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-37258][K8S][BUILD] Upgrade kubernetes-client to 5.12.0 #34939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it for Volcano?
Fix #3579: Add support for Volcano extension
There are some breaking changes in the API of k8s-cli v5.11, looks like it breaks to create httpclient with custom dispatcher like what we did here . So we need to do some changes to make it work. |
Test build #146350 has finished for PR 34939 at commit
|
Kubernetes integration test unable to build dist. exiting with code: 1 |
@dongjoon-hyun Yes. The volcano extesnion was introduced in k8s-cli v5.11. |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #146354 has finished for PR 34939 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #146360 has finished for PR 34939 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Retest this please |
Test build #146372 has finished for PR 34939 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #146373 has finished for PR 34939 at commit
|
@@ -68,6 +69,8 @@ private[spark] object SparkKubernetesClientFactory extends Logging { | |||
.getOption(s"$kubernetesAuthConfPrefix.$CLIENT_KEY_FILE_CONF_SUFFIX") | |||
val clientCertFile = sparkConf | |||
.getOption(s"$kubernetesAuthConfPrefix.$CLIENT_CERT_FILE_CONF_SUFFIX") | |||
// TODO(SPARK-37687): clean up direct usage of OkHttpClient, see also: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some backgroud on this note (also add the jira: SPARK-37687):
- There are some problems (such as IPV6 based cluster support) on okhttpclient v3, but it's a little bit diffcult to upgrade to v4.
- Kubernetes client are also consider to support other clients rather than single okhttpclient.
- Kubernetes client add a abstract layer to support variety httpclient, okhttp client as one of supported http clients.
So, we'd better to consider to cleanup okhttpclient direct usage in some right time and use the httpclient which kubernetes client diret supported to reduce the potential risk in future upgrade.
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #146386 has finished for PR 34939 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #146389 has finished for PR 34939 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm a little reluctant due to the breaking change. Shall we hold on and see the next release?
@dongjoon-hyun Thanks for review. I can understand your concerns for sure, it makes sense to wait for next release to avoid pontential flaky factor of breaking changes. But there is one thing I need to clarify, even in the next version we will still face these changes to make okhttpclient work. BTW, these changes are changed according to fabric8io/kubernetes-client#3663 (comment) . |
@Yikun Do you have a time line for the 'Cleanup direct usage of OkHttpClient' ? |
@klaus-xiong I think the deadline depends on when new better httpclient to replace okhttpclient are supported in k8s client, and also this replacement meet the requirement of Spark on K8S. It bring some potential issue when k8s-cli to stop okhttpclient support (as fabric8io/kubernetes-client#3547 mentioned, OkHttpClient has some known issue and it's not easy to upgrade OkHttpClient), it may not be a good choice if we still keep the direct OkHttpClient usage in Spark. |
1bf6aac
to
65118f6
Compare
### What changes were proposed in this pull request? This PR aims to upgrade K8s client library to 5.10.2 from 5.10.1. ### Why are the changes needed? This is a maintenance version upgrade which brings the following fix. - fabric8io/kubernetes-client@4451030 Please note that this is different from #34939 (5.11.1) which has a breaking change. In addition, the above bug fix is in `5.10.2` and `5.11.2`, not in `5.11.1`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CI. Closes #35182 from dongjoon-hyun/SPARK-37884. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
88a83c3
to
444ccae
Compare
### What changes were proposed in this pull request? This PR aims to upgrade K8s client library to 5.10.2 from 5.10.1. ### Why are the changes needed? This is a maintenance version upgrade which brings the following fix. - fabric8io/kubernetes-client@4451030 Please note that this is different from apache#34939 (5.11.1) which has a breaking change. In addition, the above bug fix is in `5.10.2` and `5.11.2`, not in `5.11.1`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CI. Closes apache#35182 from dongjoon-hyun/SPARK-37884. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience. Could you update this PR with 5.12.0? After that, I believe it's time to merge.
Update Fabric8 Kubernetes Model to v1.23.0
@dongjoon-hyun Sure, will upgrade soon. Thanks! |
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @Yikun .
Merged to master for Apache Spark 3.3.
What changes were proposed in this pull request?
This patch aims to upgrade
kubernetes-client
from 5.10.2 to 5.12.0Hightlight changes:
There are also some changes to make sure it is compatiable to create http client with custom dispatcher according to fabric8io/kubernetes-client#3663 (comment) suggestion.
Why are the changes needed?
This will bring several bug fixes and improvements (such as volcano support, Fabric8 Kuberentes model 1.23 support), see more in:
Does this PR introduce any user-facing change?
No.
How was this patch tested?